chore: setup testing libraries and tests for App.tsx#55
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
.github/workflows/test.yml
Outdated
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 8 |
There was a problem hiding this comment.
we use 9 in dogfood (probably soon to be 10)
vitest.config.ts
Outdated
| test: { | ||
| globals: true, | ||
| environment: 'happy-dom', | ||
| // setupFiles: ['./src/test/setup.ts'], |
There was a problem hiding this comment.
| // setupFiles: ['./src/test/setup.ts'], |
vitest.config.ts
Outdated
| coverage: { | ||
| provider: 'v8', | ||
| reporter: ['text', 'json', 'html'], | ||
| exclude: [ | ||
| 'node_modules/', | ||
| 'src/test/', | ||
| '**/*.d.ts', | ||
| '**/*.config.*', | ||
| 'src/vite-env.d.ts' | ||
| ] | ||
| }, |
There was a problem hiding this comment.
what are we doing with the coverage information? do we want to upload it somewhere?
There was a problem hiding this comment.
Currently nothing, a lot of the initial config was from trying to use AI to help me get something working that I eventually ditched. Sorry for forgetting to remove it from the PR.
vitest.config.ts
Outdated
| resolve: { | ||
| alias: { | ||
| '@': path.resolve(__dirname, './src'), | ||
| "monaco-editor": "./src/__mocks__/monaco-editor.ts" |
There was a problem hiding this comment.
the docs recommend using test.alias rather than resolve.alias for mocks. I'm not sure that there's any mechanical different, but the separation of concerns does seem kind of nice.
vitest.config.ts
Outdated
| 'src/vite-env.d.ts' | ||
| ] | ||
| }, | ||
| // Mock WebAssembly for tests |
| "vite-plugin-vercel": "^9.0.7", | ||
| "vitest": "^3.2.4" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
My editor removes trailing new lines, is there a reason to have it?
There was a problem hiding this comment.
short answer: yes.
long answer: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
There was a problem hiding this comment.
I'm having some issues with getting helix not to strip the new line. But this will be fixed once this PR is merged
There was a problem hiding this comment.
hmm. the docs claim that insert-final-newline defaults to true, and it doesn't seem like you've had this issue on other files. weird.
|
|
||
| - name: Install dependencies | ||
| run: pnpm install | ||
|
|
There was a problem hiding this comment.
| - name: Install dependencies | |
| run: pnpm biome check | |
tho eslint is also in here I guess? do we need both or can we get rid of eslint?
There was a problem hiding this comment.
I'm not using eslint in this. I want to do another PR where I set up knip and clean up the dev dependencies
There was a problem hiding this comment.
This is said issues and I think I'll make it a fast follow to this PR. Even including the lint check in the PR is going to bloat the diff
| import { ThemeProvider } from "@/client/contexts/theme"; | ||
| import attachGPUExample from "@/examples/code/attach-gpu.tf?raw"; | ||
| import defaultExample from "@/examples/code/default.tf?raw"; | ||
| import { initWasm } from "@/utils/wasm"; |
There was a problem hiding this comment.
does this actually work? you're importing it before the mock is set up (I always avoid mocks because this sort of thing is so confusing to me)
There was a problem hiding this comment.
Yeah because vitest moves all mocks to the top of the file
src/client/App.test.tsx
Outdated
| it("should not show the loading modal after the wasm has been loaded", async () => { | ||
| render(<TestApp />); | ||
|
|
||
| await waitFor(async () => { | ||
| expect(initWasm).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| expect(screen.queryByTestId("wasm-loading-modal")).toBeNull(); | ||
| expect(screen.queryByTestId("wasm-error-modal")).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
I would have more confidence in this test if it was merged with "should show the loading modal while the wasm module is loading"
this is all kind of one behavior. it should show when it's loading, and shouldn't when it's not. seeing that it does in fact go away after being shown is the characteristic we'd really like to see here, but all this checks is that it's not their now. it might've never been, the test never asserts that.
vitest.config.ts
Outdated
| @@ -0,0 +1,39 @@ | |||
| /// <reference types="vitest" /> | |||
| import { defineConfig } from 'vite' | |||
There was a problem hiding this comment.
looks like this file is formatted differently from our usual "double quotes with semicolons"
package.json
Outdated
| "vite-plugin-vercel": "^9.0.7", | ||
| "vitest": "^3.2.4" | ||
| }, | ||
| "packageManager": "pnpm@10.14.0" |
There was a problem hiding this comment.
we should get corepack use pnpm@10.14.0 to insert the hash here
.github/workflows/test.yml
Outdated
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 10.14.0 |
There was a problem hiding this comment.
and speaking of corepack, I wonder if we should just lean on it here. at the very least, the docs for this action say that it'll infer version from package.json and we should lean on that. https://github.com/pnpm/action-setup?tab=readme-ov-file#version
vitest.config.ts
Outdated
| export default defineConfig({ | ||
| plugins: [react()], | ||
| test: { | ||
| globals: true, |
There was a problem hiding this comment.
you're using imports, not globals. can we remove this?
| globals: true, |
vitest.config.ts
Outdated
| test: { | ||
| globals: true, | ||
| environment: "happy-dom", | ||
| css: true, |
There was a problem hiding this comment.
is this one necessary? config files tend to bloat over time, so I'd love to keep it as lean as we can to begin with.
Sets up vitest and adds tests for App.tsx
Fist PR of two for #45